test/onboard.test.js (1)

1977-1999: These drift tests grep source text instead of exercising the behavior.

They can miss runtime regressions and also fail on harmless helper/log renames. Please reuse the existing spawn-based harness patterns in this file to assert actual createSandbox() outcomes for provider-only drift, model drift, and unreadable config.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 1977 - 1999, Replace the static
source-text grep assertions with real integration tests that spawn the
onboarding binary and assert createSandbox() behavior: use the existing
spawn-based harness pattern already present in this test suite to run scenarios
that trigger provider-only drift, model drift, and unreadable config; invoke the
code paths that call getSelectionDrift and confirmRecreateForSelectionDrift,
then assert the process output/exit codes and that createSandbox was actually
recreated or left unchanged as expected (including interactive prompt behavior
for confirmRecreateForSelectionDrift), rather than matching log helper text.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

bin/lib/onboard.js (1)

950-963: ⚠️ Potential issue | 🟠 Major

Unreadable selection config is still treated as “no drift.”

At Line 951, readSandboxSelectionConfig() can return null; at Line 962 this currently resolves to changed: false, which can silently reuse a sandbox when provider/model state is actually unknown.

🔧 Proposed fix

 function getSelectionDrift(sandboxName, requestedProvider, requestedModel) {
   const existing = readSandboxSelectionConfig(sandboxName);
+  if (existing === null) {
+    return {
+      changed: true,
+      providerChanged: false,
+      modelChanged: false,
+      existingProvider: null,
+      existingModel: null,
+      unknown: true,
+    };
+  }
   const existingProvider =
     existing && typeof existing.provider === "string" ? existing.provider : null;
   const existingModel = existing && typeof existing.model === "string" ? existing.model : null;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 950 - 963, getSelectionDrift currently
treats a missing or unreadable selection config as "no drift"; change it so that
when readSandboxSelectionConfig(sandboxName) returns null/invalid, the function
treats the state as drifted. Specifically, in getSelectionDrift (and using the
existing variables existing, existingProvider, existingModel, providerChanged,
modelChanged), detect if existing is falsy or has non-string provider/model and
set the returned changed flag to true (and/or set providerChanged/modelChanged
appropriately) so we don't silently reuse a sandbox when the selection config is
unknown.

test/onboard.test.js (1)

1977-1999: Prefer behavior-driven assertions over source-text regex for drift logic.

These checks can pass even if runtime drift handling regresses but strings remain. Consider adding one execution-path test that stubs sandbox download + prompt result and asserts reuse vs recreate/abort outcomes directly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 1977 - 1999, Replace the brittle
source-text regex tests with an execution-path test that stubs the runtime
behavior: import the onboard flow (the module that calls getSelectionDrift),
stub sandbox.download (or the sandbox object used by onboard.js) to simulate an
existing sandbox, stub getSelectionDrift to return changed=true and
changed=false cases, and stub the interactive prompt used by
confirmRecreateForSelectionDrift to return yes/no; then assert the actual
outcome (that the code calls the recreate path vs the reuse/abort path) by
spying on the functions that perform recreate (e.g., sandbox.recreate or the
recreate handler) and reuse (e.g., sandbox.connect) — exercise both changed=true
with prompt=confirm (expect recreate) and changed=true with prompt=abort (expect
abort/no recreate) and changed=false (expect reuse), so tests validate runtime
behavior rather than matching strings.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/onboard.test.js (1)

2209-2218: ⚠️ Potential issue | 🟡 Minor

Test may pass spuriously when NEMOCLAW_NON_INTERACTIVE is set in CI.

The spawn env spreads process.env without explicitly clearing NEMOCLAW_NON_INTERACTIVE. If the test runner sets this variable (common in CI), the spawned script inherits it, skips the prompt entirely, and the test passes without exercising the interactive confirmation path.

🛡️ Proposed fix: explicitly clear the env variable

     const result = spawnSync(process.execPath, [scriptPath], {
       cwd: repoRoot,
       encoding: "utf-8",
       env: {
         ...process.env,
         HOME: tmpDir,
         PATH: `${fakeBin}:${process.env.PATH || ""}`,
+        NEMOCLAW_NON_INTERACTIVE: "",
       },
       timeout: 12000,
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 2209 - 2218, The spawned child inherits
process.env which can contain NEMOCLAW_NON_INTERACTIVE from CI, causing the
interactive branch to be skipped; modify the test to explicitly clear that
variable when building the env for spawnSync (change the env passed to spawnSync
for the invocation that uses
spawnSync/process.execPath/scriptPath/tmpDir/fakeBin to set
NEMOCLAW_NON_INTERACTIVE to undefined or remove it from the copied env before
adding HOME and PATH) so the child always runs in interactive mode for this
test.

test/onboard.test.js (1)

2784-2793: ⚠️ Potential issue | 🟡 Minor

Clear the recreate/non-interactive env flags for this interactive subprocess.

This case inherits process.env verbatim, unlike the nearby interactive tests. If the runner already has NEMOCLAW_NON_INTERACTIVE or NEMOCLAW_RECREATE_SANDBOX set, the confirmation path is skipped and this test no longer covers the decline flow.

🔧 Suggested fix

-    const result = spawnSync(process.execPath, [scriptPath], {
-      cwd: repoRoot,
-      encoding: "utf-8",
-      env: {
-        ...process.env,
-        HOME: tmpDir,
-        PATH: `${fakeBin}:${process.env.PATH || ""}`,
-      },
-      timeout: 12000,
-    });
+    const env = {
+      ...process.env,
+      HOME: tmpDir,
+      PATH: `${fakeBin}:${process.env.PATH || ""}`,
+    };
+    delete env.NEMOCLAW_NON_INTERACTIVE;
+    delete env.NEMOCLAW_RECREATE_SANDBOX;
+    const result = spawnSync(process.execPath, [scriptPath], {
+      cwd: repoRoot,
+      encoding: "utf-8",
+      env,
+      timeout: 12000,
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 2784 - 2793, The spawned subprocess in the
test (the spawnSync call that runs scriptPath with cwd repoRoot and env built
from ...process.env, HOME: tmpDir and PATH: `${fakeBin}:${process.env.PATH ||
""}`) unintentionally inherits NEMOCLAW_NON_INTERACTIVE and
NEMOCLAW_RECREATE_SANDBOX from the parent; explicitly clear those flags in the
env you pass to spawnSync (e.g., set NEMOCLAW_NON_INTERACTIVE and
NEMOCLAW_RECREATE_SANDBOX to undefined or an empty string in the env object) so
the interactive confirmation path is exercised and the decline flow is tested.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

docs/reference/commands.md (1)

109-112: Minor passive voice construction.

Line 111 uses passive voice: "if the stored selection cannot be read". Consider active voice for consistency with the style guide.

✏️ Suggested rewrite

-In non-interactive mode, NemoClaw recreates automatically when the stored selection is readable and differs; if the stored selection cannot be read, NemoClaw reuses by default.
+In non-interactive mode, NemoClaw recreates automatically when the stored selection is readable and differs; if NemoClaw cannot read the stored selection, it reuses by default.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/reference/commands.md` around lines 109 - 112, Change the passive clause
"if the stored selection cannot be read" to active voice for consistency; update
the sentence in docs/reference/commands.md to something like "if NemoClaw cannot
read the stored selection, NemoClaw reuses by default" (ensure the phrase still
flows with the surrounding sentences mentioning interactive mode and
NEMOCLAW_RECREATE_SANDBOX).

src/lib/onboard.ts (1)

811-817: Consider explicitly setting unknown: false for consistency.

The return object lacks the unknown property when drift comparison succeeds. While the check at line 2425 (!selectionDrift.unknown) works correctly because !undefined === true, explicitly setting unknown: false would make the type shape consistent and the intent clearer.

Suggested change

   return {
     changed: providerChanged || modelChanged,
     providerChanged,
     modelChanged,
     existingProvider,
     existingModel,
+    unknown: false,
   };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 811 - 817, The returned drift object should
include unknown: false explicitly so its shape is consistent; update the return
in the function that currently returns { changed: providerChanged ||
modelChanged, providerChanged, modelChanged, existingProvider, existingModel }
to also include unknown: false, ensuring code that checks
!selectionDrift.unknown (referenced elsewhere) sees an explicit boolean instead
of undefined.